Add size_hints for btree iterators#49550
Add size_hints for btree iterators#49550Phlosioneer wants to merge 2 commits intorust-lang:masterfrom
Conversation
Continues work on adding size_hint implementations for iterators: issue rust-lang#49205
src/liballoc/btree/map.rs
Outdated
| // from a NodeRef. | ||
| (1, None) | ||
| } | ||
| } |
There was a problem hiding this comment.
What is the utility of a size hint like this? Would it be better to just not define it at all?
There was a problem hiding this comment.
I think it's useful to have it, because it may be implemented in the future. size_hint is easily forgotten unless it's in the code.
There was a problem hiding this comment.
I'm not sure I buy that form of reasoning.
There was a problem hiding this comment.
In this particular case I buy it because (1, None) in some cases is still more information than the default size_hint of (0, None).
But #49552 contains some size_hint implementations that literally just return (0, None). For those cases I don't think I buy it.
There was a problem hiding this comment.
Well in particular, the bit about "because it may be implemented in the future." I'd rather not add speculative impls?
There was a problem hiding this comment.
@BurntSushi would just adding // FIXME(#49205): Add size_hint be better? I just want to make sure it's not forgotten.
There was a problem hiding this comment.
@Ixrec I don't know what you're talking about? None of the implementations I wrote return (0, None), or even this controversial (_, None)... Please comment on that PR if you see any specific issues, maybe you see something I don't.
There was a problem hiding this comment.
The other option is to add a length field like every other BTree iterator. But that isn't a trivial thing, which is what this PR is for.
There was a problem hiding this comment.
I'm just going to move forward with a FIXME. There's no consensus about what to do.
src/liballoc/btree/map.rs
Outdated
| let upper = match (left_upper, right_upper) { | ||
| (Some(left), Some(right)) => left.checked_add(right), | ||
| (left, None) => left, | ||
| (None, right) => right |
There was a problem hiding this comment.
None as the upper is infinity, so I think this is wrong.
Also, this lift can often be written nicely with ?-on-Option, something like
(||{ left_upper?.checked_add(right_upper?) })()There was a problem hiding this comment.
Yes, you are right! I got it backwards.
I don't think it's worth it to create a closure purely to use ?... it doesn't make it any more readable. IMHO a closure that is immediately called is harder to read & maintain.
There was a problem hiding this comment.
Well, it'll soon be do catch { left_upper?.checked_add(right_upper?)? } to avoid the IIFE (that I agree is ugly), but please don't do that until #49371 merges.
There was a problem hiding this comment.
That's still a lot of ?. But much better. I'll be sure to make that change whenever it merges.
emilyalbini
added
the
S-waiting-on-review
BurntSushi
added
S-waiting-on-team
and removed
S-waiting-on-review
|
Resolved waiting-on-team by just replacing the controversial code with a FIXME. See #49550 (comment) |
TimNN
added
S-waiting-on-review
|
Ping from triage @BurntSushi! This PR needs your review! |
|
I'm not too sure what else needs to happen here. I'm not sure I agree with adding size hints that aren't actually used. cc @rust-lang/libs @alexcrichton |
|
Ah yeah I think it'd be best to hold off on landing this until it's a public interface and tested as well. |
|
This PR has been pretty thoroughly gutted. I think it's possible to refactor things so that Until then, I'm just going to close this. |
7 participants
Continues work on adding size_hint implementations for iterators:
issue #49205